TypeCheck now can preserve AnnotateTypeExpr to prevent invalid re-typing by delphamk · Pull Request #164903 · cockroachdb/cockroach

delphamk · 2026-03-05T07:12:36Z

Only discard AnnotateTypeExpr when its type is redundant.

Example: 'min(NULL:::FLOAT)' now resolves to type FLOAT instead of STRING.

Epic: none

trunk-io · 2026-03-05T07:12:40Z

Merging to master in this repository is managed by Trunk.

To merge this pull request, check the box to the left or comment /trunk merge below.

blathers-crl · 2026-03-05T07:12:41Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2026-03-05T07:12:48Z

This change is

mw5h

Review

The core idea is sound — preserving type annotations on NULL to prevent downstream type inference from losing the user's intent. However, there are a few issues to address.

1. Hand-edited generated file (blocking)

pkg/sql/sem/tree/eval_expr_generated.go is explicitly marked:

// Code generated by eval_gen.go. DO NOT EDIT.
// Regenerate this file with the command:
//   ./dev generate go

The PR hand-edits this file to add EvalAnnotateTypeExpr to the ExprEvaluator interface and the Eval method on *AnnotateTypeExpr. The correct approach is:

Add the typeAnnotation embedding to AnnotateTypeExpr in expr.go (which the PR does)
Run ./dev generate go to regenerate eval_expr_generated.go

The hand-edits will likely cause CI failures or be overwritten.

2. Logic condition in `TypeCheck` may be fragile

The new condition at type_check.go:817:

if subExpr.ResolvedType() != types.Unknown || annotateType.IsWildcardType() || cast.ValidCast(annotateType, desired, cast.ContextImplicit) {
    return subExpr, nil
}

The ValidCast(annotateType, desired, ...) check uses the desired type, which is the type the caller wants (e.g., what an aggregate function expects). This means the behavior of ::: depends on surrounding context, which seems fragile. Consider: what if desired is types.Any? ValidCast to Any may always succeed, stripping the annotation even when it should be preserved.

A clearer condition might be: "preserve the annotation only when the inner expression is Unknown and the annotated type differs from Unknown."

3. `buildScalar` handling may be too narrow

In scalar.go, the new case only preserves type for DNull:

case *tree.AnnotateTypeExpr:
    texpr := t.TypedInnerExpr()
    if texpr == tree.DNull {
        out = b.factory.ConstructConstVal(tree.DNull, t.ResolvedType())
    } else {
        return b.buildScalar(texpr, inScope, outScope, outCol, colRefs)
    }

Could there be cases where the wrapper is preserved but the inner expression isn't literally DNull? For example, an expression that resolves to Unknown type but isn't the DNull constant. In that case, the else branch would silently discard the annotation.

4. Missing broader test coverage

The tests cover the optimizer builder (build testdata), but there are no end-to-end SQL tests verifying the actual query result. For instance, a logic test confirming:

SELECT pg_typeof(min(NULL:::FLOAT))  -- should return 'double precision'

This would catch regressions at the execution level, not just the plan level.

5. nit: comment style

// return subExpr if the AnnotateTypeExpr is redundant

Should be a complete sentence per Go conventions: // Return subExpr if the AnnotateTypeExpr is redundant.

mw5h

Turns out even AI can double-post.

This patch is an interesting idea, but it's going to need a little more work before it's ready to merge.

delphamk · 2026-03-09T22:08:59Z

I appreciate your comments. I’ve provided an explanation of my reasoning for the TypeCheck change and hope it’s understandable.

1) Fixed and caused by a formatting issue in pkg/sql/sem/eval/expr.go
2) TypeCheck logic

subExpr.ResolvedType() != types.Unknown

subExpr is Unknown and 'This type should never be returned for an expression that does not *always* \ evaluate to NULL.'
This means duplicate calls to subExpr.TypeCheck(...) will resolve to annotateType at a minimum. Implicit casting could change its type, but that is valid.
It is still possible for the annotation to be striped when subExpr is Unknown and is covered in the following or checks.

annotateType.IsWildcardType()

This can happen for NULL:::RECORD. When the annotation is preserved the otherwise NULL value is not reTyped to a tuple as it would normally. I think that AnyTuple requires some changes for this. For example SELECT CASE WHEN true THEN ('a', 2) ELSE NULL::RECORD END will error, where NULL does not.
A better check here may be annotateType.IsAmbiguous because "IsAmbiguous returns true if this type is in UnknownFamily or AnyFamily."
In other words, this checks if the the annotation allows NULL/Unknown, and if true, the subExpr has resolved as expected.

ValidCast(annotateType, desired, ...)

This is intended to cover cases like NULL:::INT:::FLOAT , where :::INT can be discarded since upstream implicitly desires a FLOAT.

what if desired is types.Any?

ValidCast will return false unless if annotateType is also types.Any. This may be a confusion with if desired is types.Unknown, which always return true.

Simply, this change is looking to fix cases like min(NULL:::FLOAT).
The subExpr (stripped to NULL) will be cast to String as a way to reduce the number of overloads.
Considering that Annotations have been returning the subExpr up to this point, the logic could be narrowed down to: !(subExpr.ResolvedType() != types.Unknown && desired != types.AnyElement) without breaking existing logic.

3) buildScalar logic
For this the only cases to slip though would be types.Unknown and !IsWildcardType.
I updated the logic to check unknown type instead and then cast to its type.

4) Added some more cases but can add more if needed.
For test case you mentioned, currently it is unknown which is the same as SELECT pg_typeof(min(NULL::FLOAT));

5) Updated the comment but can make it more verbose if needed.

Regarding my comments on RECORD casting, I have an example of what this might look like here. With this change, the IsAmbiguous should not be needed.

blathers-crl · 2026-03-09T22:09:22Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

blathers-crl · 2026-03-09T22:12:34Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

Only discard AnnotateTypeExpr when its type is redundant. Example: 'min(NULL:::FLOAT)' now resolves to type FLOAT instead of STRING. Release note (sql change): Preserve type annotations on NULL expressions so aggregate functions like min() resolve to the annotated type rather than STRING. Epic: none

blathers-crl · 2026-06-28T19:59:26Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

delphamk · 2026-06-28T20:04:29Z

Pushed an updated revision with rebase.

Please let me know if there are any other comments. I plan on using this commit in another PR and will reference this for context.

I agree that the check for 2) does not cover the full condition but it still tightens NULL annotations. As I mentioned, NULL:::RECORD is not covered yet. This change instead covers most missing NULL annotations without reducing previous cases.

delphamk requested a review from a team as a code owner March 5, 2026 07:12

delphamk requested review from mw5h and removed request for a team March 5, 2026 07:12

blathers-crl Bot added the O-community Originated from the community label Mar 5, 2026

mw5h reviewed Mar 6, 2026

View reviewed changes

delphamk force-pushed the preserveAnnotations branch from 18d634f to 3d14d65 Compare March 9, 2026 22:09

delphamk force-pushed the preserveAnnotations branch from 3d14d65 to b611bb4 Compare March 9, 2026 22:12

delphamk force-pushed the preserveAnnotations branch from b611bb4 to f3fed65 Compare June 28, 2026 19:59

delphamk marked this pull request as draft July 2, 2026 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

TypeCheck now can preserve AnnotateTypeExpr to prevent invalid re-typing#164903

TypeCheck now can preserve AnnotateTypeExpr to prevent invalid re-typing#164903
delphamk wants to merge 1 commit into
cockroachdb:masterfrom
delphamk:preserveAnnotations

delphamk commented Mar 5, 2026

Uh oh!

trunk-io Bot commented Mar 5, 2026

Uh oh!

blathers-crl Bot commented Mar 5, 2026

Uh oh!

cockroach-teamcity commented Mar 5, 2026

Uh oh!

mw5h left a comment

Uh oh!

mw5h left a comment •

edited

Loading

Uh oh!

delphamk commented Mar 9, 2026

Uh oh!

blathers-crl Bot commented Mar 9, 2026

Uh oh!

blathers-crl Bot commented Mar 9, 2026

Uh oh!

blathers-crl Bot commented Jun 28, 2026

Uh oh!

delphamk commented Jun 28, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

delphamk commented Mar 5, 2026

Uh oh!

trunk-io Bot commented Mar 5, 2026

Uh oh!

blathers-crl Bot commented Mar 5, 2026

Uh oh!

cockroach-teamcity commented Mar 5, 2026

Uh oh!

mw5h left a comment

Choose a reason for hiding this comment

Review

1. Hand-edited generated file (blocking)

2. Logic condition in TypeCheck may be fragile

3. buildScalar handling may be too narrow

4. Missing broader test coverage

5. nit: comment style

Uh oh!

mw5h left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delphamk commented Mar 9, 2026

Uh oh!

blathers-crl Bot commented Mar 9, 2026

Uh oh!

blathers-crl Bot commented Mar 9, 2026

Uh oh!

blathers-crl Bot commented Jun 28, 2026

Uh oh!

delphamk commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

2. Logic condition in `TypeCheck` may be fragile

3. `buildScalar` handling may be too narrow

mw5h left a comment •

edited

Loading

delphamk commented Jun 28, 2026 •

edited

Loading